-
Notifications
You must be signed in to change notification settings - Fork 1
Implement hero deletion endpoint with confirmation requirement #61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: 0GiS0 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a new DELETE endpoint to safely remove heroes by requiring explicit confirmation, checking existence, logging actions, and returning clear status codes and messages.
- Implements
deleteHeroinHeroServicewith confirmation, existence check, deletion, and audit logging. - Exposes
DELETE /api/heroes/:idinHeroControllerwith ID validation, confirmation processing, and proper HTTP responses. - Updates routing, HTTP example scripts, and documentation to cover deletion use cases.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/hero.service.ts | Defined DeleteHeroResult and added deleteHero method with safety checks |
| src/controllers/hero.controller.ts | Introduced deleteHero action handling confirmation and status codes |
| src/routes/hero.routes.ts | Added DELETE route for hero deletion |
| heroes.http | Added HTTP examples for deletion scenarios |
| docs/basic-usage.md | Documented delete endpoint usage, parameters, and responses |
Comments suppressed due to low confidence (1)
src/services/hero.service.ts:163
- No unit tests cover the new deleteHero logic; consider adding tests for confirmation missing, hero not found, successful deletion, and error handling.
async deleteHero(id: number, confirm: boolean): Promise<DeleteHeroResult> {
| await HeroModel.deleteOne({ id }); | ||
|
|
||
| // Log the deletion for audit purposes | ||
| console.log(`Hero deleted: ${hero.name} (ID: ${id}) at ${new Date().toISOString()}`); |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using the project's structured logger instead of console.log for audit logging to maintain consistency and enable log level control.
| message: `Hero ${hero.name} has been deleted`, | ||
| }; | ||
| } catch (error) { | ||
| console.error(`Error deleting hero with id ${id}:`, error); |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using the project's structured logger instead of console.error for error logging to maintain consistency and facilitate log aggregation.
| console.error(`Error deleting hero with id ${id}:`, error); | |
| logger.error(`Error deleting hero with id ${id}:`, { id, error }); |
| // Determine appropriate status code based on error | ||
| if (result.error === 'Confirmation is required to delete a hero') { | ||
| res.status(400).json({ error: result.error }); | ||
| } else if (result.error?.includes('not found')) { | ||
| res.status(404).json({ error: result.error }); | ||
| } else { | ||
| res.status(500).json({ error: result.error }); |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching on exact error message content is brittle; consider extending DeleteHeroResult with a status code or error code enum to drive HTTP status decisions.
| // Determine appropriate status code based on error | |
| if (result.error === 'Confirmation is required to delete a hero') { | |
| res.status(400).json({ error: result.error }); | |
| } else if (result.error?.includes('not found')) { | |
| res.status(404).json({ error: result.error }); | |
| } else { | |
| res.status(500).json({ error: result.error }); | |
| // Determine appropriate status code based on errorCode | |
| switch (result.errorCode) { | |
| case 'INVALID_CONFIRMATION': | |
| res.status(400).json({ error: 'Confirmation is required to delete a hero' }); | |
| break; | |
| case 'HERO_NOT_FOUND': | |
| res.status(404).json({ error: 'Hero not found' }); | |
| break; | |
| default: | |
| res.status(500).json({ error: 'An unknown error occurred' }); |
| } | ||
|
|
||
| ### Delete hero without confirmation (will fail) | ||
| DELETE {{baseUrl}}/heroes/1 HTTP/1.1 |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test request path is missing the /api prefix; it should be {{baseUrl}}/api/heroes/1 to match the configured route.
This PR implements the hero deletion functionality for the Tour of Heroes API with several key safety features:
Implementation Details
Added a
deleteHeromethod toHeroServicethat:Added a controller method in
HeroControllerthat:Updated routes to include the new DELETE endpoint
Added comprehensive documentation:
basic-usage.mdheroes.httpfor testingExample Usage
Safety Features
confirm=truequery parameter to prevent accidental deletionsFixes #40.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.